Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue introduced by 7dc0faf by doing more cleanup in gboolean vs … #17537

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

TurboGit
Copy link
Member

…int.

Fixes #17536

@TurboGit TurboGit added this to the 5.0 milestone Sep 25, 2024
@TurboGit TurboGit added bugfix pull request fixing a bug priority: high core features are broken and not usable at all, software crashes scope: image processing correcting pixels scope: DAM managing files, collections, archiving, metadata, etc. labels Sep 25, 2024
@TurboGit
Copy link
Member Author

@wpferguson : Can you double check this. TIA.

@wpferguson
Copy link
Member

wpferguson commented Sep 25, 2024

I saw this last night, but didn't realize that it was my change that caused it. When I went to find the problem, I couldn't reproduce. Testing now....

I still see it on tif, dng, and a cr3. I'll do some digging....

Another datapoint, opened a skull image in darkroom and it's fine, but the filmstrip thumbnails are all skulls

@wpferguson
Copy link
Member

I backed up before my change and loaded some tif's with -d imageio -d cache

Here's part of the log...

     7.1830 [tiff_open] 3089x2048 8bpp, 3 samples per pixel.
     7.1888 [tiff_open] 3089x2048 8bpp, 3 samples per pixel.
     7.1892 [tiff_open] 3089x2048 8bpp, 3 samples per pixel.
     7.1898 [tiff_open] 2545x3393 16bpp, 3 samples per pixel.
     7.1902 [tiff_open] 1729x1729 16bpp, 3 samples per pixel.
     7.1906 [tiff_open] 4816x3219 16bpp, 4 samples per pixel.
     7.2731 [dt_imageio_export_with_flags]  modules: colorin colorout gamma
     7.2731 [dt_imageio_export] thumbnail imgid 38474, 3089x2048 --> 1357x900 (scale=0.4395, maxscale=1.0000). upscale=no, hq=no
     7.2785 [dt_imageio_export_with_flags]  modules: colorin colorout gamma
     7.2786 [dt_imageio_export] thumbnail imgid 38475, 3089x2048 --> 1357x900 (scale=0.4395, maxscale=1.0000). upscale=no, hq=no
     7.3014 [dt_imageio_export_with_flags]  modules: colorin colorout gamma
     7.3014 [dt_imageio_export] thumbnail imgid 38476, 3089x2048 --> 1357x900 (scale=0.4395, maxscale=1.0000). upscale=no, hq=no
     7.3149 [dt_imageio_export_with_flags]  modules: colorin colorout gamma
     7.3149 [dt_imageio_export] thumbnail imgid 38477, 2545x3393 --> 675x900 (scale=0.2653, maxscale=1.0000). upscale=no, hq=no
     7.3184 [mipmap_cache] generate mip 3 for ID=38474 from scratch
     7.3191 [tiff_open] 4816x3219 8bpp, 4 samples per pixel.
     7.3322 [mipmap_cache] generate mip 3 for ID=38475 from scratch
     7.3340 [tiff_open] 2550x3300 8bpp, 3 samples per pixel.
     7.3431 [mipmap_cache] generate mip 3 for ID=38476 from scratch

trying to trace the execution to see how we get from tiff open to dt_imageio_export

@wpferguson
Copy link
Member

The problem seems to be in dt_imageio_large_thumbnail but I haven't figured out what it is or how to solve it.

Unless you have an idea, I would say revert my PR and I'll work on it until I solve the problems. At least that way it wont affect people trying to use master.

@TurboGit TurboGit force-pushed the po/fix-export-and-cache branch 2 times, most recently from f41859f to 3280a1a Compare September 26, 2024 07:09
@AndDiSa
Copy link

AndDiSa commented Sep 26, 2024

For me the fix works, at least for NEF and DNG

@TurboGit
Copy link
Member Author

@AndDiSa : Indeed, it works for RAW & JPEG but still buggy for TIFF.

@wpferguson : I have pushed the revert I had prepared.

@wpferguson
Copy link
Member

After a nights sleep and some time to think this is what I came up with.

The original problem was that dt_imageio_export_with_flags is a gboolean returning "backwards" indicators. dt_imageio_export_with_flags used to return an int, 0 for success and 1 for error which is standard. Nothing downstream uses dt_imageio_export_with_flags as a boolean and all the downstream code still treats it as returning an int.

So, I propose simply turning dt_imageio_export_with_flags back into an int and let it return 0 and 1. The sanity crowd is happy because the return value is standard, and almost no code needs to be changed (I'll look at the Lua stuff).

@wpferguson
Copy link
Member

As for dt_imageio_large_thumbnail I'm still going to play with this and see if I can at least understand the coupling between it and the mipmap_cache code. I'm starting to suspect that the real issue may be the mipmap_cache not asking (or not asking correctly) for the thumbnail.

@wpferguson
Copy link
Member

Thanks for cleaning up my mess.

@TurboGit
Copy link
Member Author

TurboGit commented Sep 26, 2024

@wpferguson : No problem, I spend quite some time trying to fix the issue but could not either... One data point:

In dt_imageio_large_thumbnail we have (in this PR):

gboolean dt_imageio_large_thumbnail(const char *filename,
                                    uint8_t **buffer,
                                    int32_t *width,
                                    int32_t *height,
                                    dt_colorspaces_color_profile_type_t *color_space)
{
  int res = TRUE;

So res to TRUE, and some early return:

  // get the biggest thumb from exif
  if(dt_exif_get_thumbnail(filename, &buf, &bufsize, &mime_type))
    goto error;

But the goto error here is misleading, a whole mess :( This is not an error as we exit with 1 (TRUE) !!!

I tried fixing that but the last state of this PR is ok for everything expect TIFF.

I would say that we want to continue to work on this... We need to fix for a better future :)

@wpferguson
Copy link
Member

Maybe I should change my username to Pandora :-D

I would say that we want to continue to work on this... We need to fix for a better future :)

I agree. My brain segfaulted a few times trying to make sense of the code. I'll keep playing with it. I enjoy puzzles

@TurboGit
Copy link
Member Author

Good, start over this PR which has already quite some fixes.

* src/imageio/imageio.c - changed dt_imageio_export_with_flags to return
                        TRUE on success and FALSE on error.  Since
                        dt_imageio_export calls dt_imageio_export_with_flags
                        to do the actual export, dt_imageio_export now
                        returns TRUE for success and FALSE for error.

src/imageio/storage/disk.c
src/imageio/storage/email.c
src/imageio/storage/gallery.c
src/imageio/storage/pwigo.c
src/lua/luastorage.c         - updated the places where dt_imageio_export was
                               called to use the boolean return value for
                               success tests.
@TurboGit
Copy link
Member Author

@wpferguson : I have just pushed a new version with your changes and mine on top. Let's play :)

@TurboGit TurboGit added the wip pull request in making, tests and feedback needed label Sep 27, 2024
@TurboGit TurboGit marked this pull request as draft September 27, 2024 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix pull request fixing a bug priority: high core features are broken and not usable at all, software crashes scope: DAM managing files, collections, archiving, metadata, etc. scope: image processing correcting pixels wip pull request in making, tests and feedback needed
Projects
None yet
3 participants